Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deprecated items #13600

Merged
merged 8 commits into from
Feb 15, 2024
Merged

Remove deprecated items #13600

merged 8 commits into from
Feb 15, 2024

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Feb 9, 2024

What type of PR is this?

Cleanup

@rkapka rkapka added Cleanup Code health! v5 labels Feb 9, 2024
@rkapka rkapka requested a review from a team as a code owner February 9, 2024 14:02
@@ -173,7 +173,7 @@ func TestGetSpec(t *testing.T) {
data, ok := resp.Data.(map[string]interface{})
require.Equal(t, true, ok)

assert.Equal(t, 132, len(data))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what changed here? was this just broken?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed DeprecatedSafeSlotsToUpdateJustified and DeprecatedSafeSlotsToImportOptimistically from config/params/config.go

@@ -809,9 +735,7 @@ message GetValidatorParticipationRequest {
}
}

// DEPRECATED: Prysm Web UI and associated endpoints will be fully removed in a future hard fork.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also global participation rate should be looked at above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the e2e can be fixed regarding it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. We can't remove validator participation because it's not marked as deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validator participating is not deprecated but there are fields in it that are

float global_participation_rate = 1 [deprecated = true]; for example

@@ -809,9 +735,7 @@ message GetValidatorParticipationRequest {
}
}

// DEPRECATED: Prysm Web UI and associated endpoints will be fully removed in a future hard fork.
message ValidatorParticipationResponse {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we aren't using validator participating in webui anymore I think...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately it's not marked as deprecated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? the option deprecated = true got removed here , I don't understand why it's not marked as deprecated

james-prysm
james-prysm previously approved these changes Feb 12, 2024
@rkapka rkapka enabled auto-merge February 12, 2024 17:30
repeated Duty duties = 1 [deprecated = true];

repeated Duty current_epoch_duties = 2;
repeated Duty current_epoch_duties = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid changing message IDs.

Suggested change
repeated Duty current_epoch_duties = 1;
reserved 1; // Deprecated fields
repeated Duty current_epoch_duties = 2;


repeated Duty next_epoch_duties = 3;
repeated Duty next_epoch_duties = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid changing message IDs.

Suggested change
repeated Duty next_epoch_duties = 2;
repeated Duty next_epoch_duties = 3;

@@ -228,6 +185,7 @@ service BeaconChain {
// validator attestations. This endpoint allows for retrieval of genesis
// information via a boolean query filter.
rpc GetValidatorParticipation(GetValidatorParticipationRequest) returns (ValidatorParticipationResponse) {
option deprecated = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this newly deprecated?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we had a discussion i wanted to get rid of it but realized we don't have it deprecated yet...

proto/prysm/v1alpha1/beacon_block.proto Show resolved Hide resolved
@rkapka rkapka disabled auto-merge February 12, 2024 17:32
@james-prysm james-prysm dismissed their stale review February 12, 2024 23:03

waiting for preston's feedback

Copy link
Contributor

@saolyn saolyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intention to remove all the code marked as deprecated? I see you removed some things from eg. beacon-chain/rpc/prysm/v1alpha1/beacon/blocks.go but not others

@rkapka
Copy link
Contributor Author

rkapka commented Feb 13, 2024

Is the intention to remove all the code marked as deprecated? I see you removed some things from eg. beacon-chain/rpc/prysm/v1alpha1/beacon/blocks.go but not others

The ones I didn't remove are still used. It might be possible to rewrite these usages in terms of the HTTP event stream, but it's too late to do this now.

# Conflicts:
#	beacon-chain/rpc/prysm/v1alpha1/beacon/attestations.go
#	beacon-chain/rpc/prysm/v1alpha1/beacon/attestations_test.go
#	beacon-chain/rpc/prysm/v1alpha1/beacon/blocks.go
#	beacon-chain/rpc/prysm/v1alpha1/beacon/blocks_test.go
#	beacon-chain/rpc/prysm/v1alpha1/beacon/validators_stream.go
#	beacon-chain/rpc/prysm/v1alpha1/beacon/validators_stream_test.go
#	beacon-chain/rpc/prysm/v1alpha1/validator/duties.go
#	beacon-chain/rpc/prysm/v1alpha1/validator/duties_test.go
#	cmd/prysmctl/deprecated/BUILD.bazel
#	cmd/prysmctl/deprecated/checkpoint/BUILD.bazel
#	cmd/prysmctl/deprecated/cmd.go
#	cmd/prysmctl/main.go
#	proto/prysm/v1alpha1/beacon_block.pb.go
#	proto/prysm/v1alpha1/beacon_chain.pb.go
#	proto/prysm/v1alpha1/debug.pb.go
#	proto/prysm/v1alpha1/validator.pb.go
#	testing/mock/beacon_chain_service_mock.go
#	testing/mock/beacon_service_mock.go
#	testing/mock/beacon_validator_client_mock.go
#	testing/mock/beacon_validator_server_mock.go
@rkapka rkapka added this pull request to the merge queue Feb 15, 2024
Merged via the queue into develop with commit 6c5351c Feb 15, 2024
17 checks passed
@rkapka rkapka deleted the v5-cleanup branch February 15, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup Code health!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants